-
Notifications
You must be signed in to change notification settings - Fork 14
Rename category type field in ElectricalComponent #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename category type field in ElectricalComponent #333
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR renames a proto message and a field to improve clarity in the naming of electrical component metadata.
- Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo.
- Renames the field from category_type to category_specific_info in ElectricalComponent.
- Updates release notes to reflect these naming changes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto | Renamed message and field for improved clarity regarding electrical component metadata. |
| RELEASE_NOTES.md | Updated the release notes to document the renaming of the proto message and field. |
Comments suppressed due to low confidence (1)
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:351
- Update the comment to reflect the new naming (e.g. 'The metadata specific to the electrical component category info.') to avoid confusion with the renamed field.
// The metadata specific to the component category type.
|
You sneaked the unification of metric and metric sample with sensors, which seems like a major changeworth of mentioning in the PR (arguably more relevant than a simple rename). |
No that is a mistake. The commits should not be there. Will remove those. |
b408e0d to
4dfe689
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A PR that renames a proto message and associated field in ElectricalComponent to better reflect their purpose.
- Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo and changes the oneof field name from metadata to info.
- Updates the field name in ElectricalComponent from category_type to category_specific_info.
- Amends the release notes to document these renames.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto | Renames the proto message and field to improve clarity. |
| RELEASE_NOTES.md | Updates release notes to reflect the renaming, though with a slight inconsistency in naming. |
63837c5 to
496c781
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves naming clarity by renaming the category-specific metadata for electrical components.
- Rename the protobuf message from
ElectricalComponentCategoryMetadataVarianttoElectricalComponentCategorySpecificInfo - Update the oneof and field name from
category_typetocategory_specific_info - Reflect these renames in the release notes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto | Renamed message and updated field name and oneof |
| RELEASE_NOTES.md | Updated release notes to document the renames |
Comments suppressed due to low confidence (2)
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:321
- Update this comment to reflect the new naming, e.g. 'Category-specific info for a microgrid component.'
// Metadata specific to a microgrid component.
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:351
- Revise this comment to reference 'category-specific info' instead of 'metadata', for example: 'The category-specific info for the component.'
// The metadata specific to the component category type.
720a860 to
453b60e
Compare
| message ElectricalComponentCategoryMetadataVariant { | ||
| oneof metadata { | ||
| message ElectricalComponentCategorySpecificInfo { | ||
| oneof info { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super low importance, but while we are at this, and while researching some other oneof, I got the impression kind is pretty standard1 mainly when a message only contains one oneof and the name becomes redundant with the message name, so maybe we can use that here too?
I also asked Gemini, and they recommend component or details.
Gemini response
Okay, this is a great, concrete example to discuss! Let's look at your ElectricalComponentCategorySpecificInfo message:
// Metadata specific to a microgrid component.
message ElectricalComponentCategorySpecificInfo {
oneof info { // <-- This is the oneof in question
frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1;
frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2;
frequenz.api.common.v1.microgrid.electrical_components.Fuse fuse = 3;
frequenz.api.common.v1.microgrid.electrical_components.GridConnectionPoint grid = 4;
frequenz.api.common.v1.microgrid.electrical_components.Inverter inverter = 5;
frequenz.api.common.v1.microgrid.electrical_components.VoltageTransformer voltage_transformer = 6;
}
}The containing message is ElectricalComponentCategorySpecificInfo. This tells us that the message holds information specific to a component category. The oneof then provides the actual data for one of those specific component types.
Let's evaluate the current name info and some alternatives:
-
info(current):- Access path:
...category_specific_info.info.battery - Pros: Short.
- Cons: It's a bit redundant. "Electrical Component Category Specific Info's info" sounds like "the information of the specific information." While not terrible, it could be more distinct.
- Access path:
-
kind:- Access path:
...category_specific_info.kind.battery - Pros: This is a standard and widely understood convention for
oneoffields that act as discriminated unions, indicating the "kind" of data present. It signifies that the specific info can be of the kind "battery," "ev_charger," etc. - Cons: It's a bit abstract.
- Access path:
-
type:- Access path:
...category_specific_info.type.battery - Pros: Similar to
kind, very common. It clearly indicates that this field determines the specific type of the component information. - Cons: Also somewhat abstract.
- Access path:
-
component:- Access path:
...category_specific_info.component.battery - Pros: This is quite descriptive in your domain. The
oneofis literally selecting which specific component's detailed message is being provided. The fields within theoneofarebattery,ev_charger– these are types of components. So, sayingcategory_specific_info.componentclearly points to "the actual component data within this specific info structure." - Cons: The word "component" is already part of the message name and the broader context (
ElectricalComponent). However, this isn't necessarily a strong con, as it reinforces the domain concept.
- Access path:
-
details:- Access path:
...category_specific_info.details.battery - Pros: The
oneofholds the details for the specific category. This avoids the "info.info" redundancy and is quite intuitive. "Category-specific info's details" makes good sense. - Cons: A bit more generic than
component.
- Access path:
-
data:- Access path:
...category_specific_info.data.battery - Pros: Similar to
details. Theoneofcontains the actual data payload for the specific category. Clear and common for payload-carrying fields. - Cons: Also somewhat generic.
- Access path:
Recommendation:
Given your specific structure and domain:
-
Option A:
component
This is a strong contender. Theoneofis selecting from different component types.category_specific_info.component.batteryreads very naturally as "the battery component data within the category-specific information." It's domain-relevant and clearly indicates the nature of the choice. -
Option B:
detailsordata
These are also good choices.category_specific_info.details.batteryemphasizes that thisoneofprovides the specific, detailed information for the category. It elegantly avoids the redundancy of the originalinfoname. -
Option C:
kindortype
These are safe, standard choices that any Protobuf developer would immediately understand. They emphasize the polymorphic nature of theElectricalComponentCategorySpecificInfomessage.
My top recommendation would be component.
// Metadata specific to a microgrid component.
message ElectricalComponentCategorySpecificInfo {
oneof component { // Renamed from 'info' to 'component'
frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1;
frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2;
// ... and so on
}
}This feels the most direct and semantically aligned with what the oneof represents: the specific component type whose data is included.
Second choice would be details:
// Metadata specific to a microgrid component.
message ElectricalComponentCategorySpecificInfo {
oneof details { // Renamed from 'info' to 'details'
frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1;
frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2;
// ... and so on
}
}This is also very clear and avoids any redundancy.
Ultimately, the best choice is one that is clear to your team and consistent with other oneof naming in your project. However, based on the information provided, component seems to offer the best balance of domain-specificity and clarity for this particular oneof. info is acceptable but less ideal than these alternatives.
Footnotes
-
For example: https://github.com/protocolbuffers/protobuf/blob/c4ec2a8c0e00f3107a3aa6186e9a1e490b5bca8b/src/google/protobuf/struct.proto#L62-L78 although when searching more in depth I actually didn't find soooo many uses of
kind. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, will change to kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Ready for another look.
453b60e to
59a655e
Compare
This commit renames the folowing: - `ComponentCatgoryMetadataVariant` to `ElectricalComponentCategorySpecificInfo` - `ElectricalComponent.category_type` to `ElectricalComponent.category_specific_info` These renames make the entity names better reflect their purpose. Signed-off-by: Tiyash Basu <[email protected]>
59a655e to
27fedfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the naming for electrical component category metadata to better reflect its purpose.
- Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo.
- Renames the ElectricalComponent field from category_type to category_specific_info.
- Updates the release notes accordingly.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto | Renamed message and field to clarify component metadata. |
| RELEASE_NOTES.md | Updated release notes to document the renaming changes. |
Comments suppressed due to low confidence (1)
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:381
- The comment still references 'component category type'; consider updating it to 'component category specific info' to reflect the renaming.
// The metadata specific to the component category type.
This commit renames the folowing:
ComponentCatgoryMetadataVarianttoElectricalComponentCategorySpecificInfoElectricalComponent.category_typetoElectricalComponent.category_specific_infoThese renames make the entity names better reflect their purpose.